feat: add field to never drop data for a customer #17365
Conversation
raquelmsmith
changed the title
|
@benjackwhite can you review again for my cache usage? |
…thog into feat/never-drop-data
posthog/tasks/email.py
Outdated
| def send_over_quota_but_not_dropped_email_to_cs(organization_id: int) -> None: | ||
| organization: Organization = Organization.objects.get(pk=organization_id) | ||
| message = EmailMessage( | ||
| campaign_key=f"over_quota_but_not_dropped-{organization_id}-{timezone.now().timestamp()}", |
There was a problem hiding this comment.
Could we perhaps use the campaign key to achieve the caching? If we make the key use the calendar week like now().isocalendar().week mixed with the year, then we will only send one message per week which achieves the caching mechanism and makes it easier to debug as we can easily inspect and modify postgres, whereas redis is a bit more transient.
Or alternatively extend the MessagingRecord to support multiple emails being sent for the same campaign_key with some sort of ttl. Like
message.send_again_after = timedelta(days=7)
if message.send_again_after and last_message.sent_at < now() - message.send_again_after:
There was a problem hiding this comment.
If we make the key use the calendar week like now().isocalendar().week mixed with the year, then we will only send one message per week
Kind of... if a message is sent the last day of week 10, then another one will be sent the next day since it's week 11. So I don't think this is the right solution.
Or alternatively extend the MessagingRecord to support multiple emails being sent for the same campaign_key with some sort of ttl.
I can see other situations where we want to resend emails and have a record of the same exact email being sent on different days. I'll extend it in this way.
|
@raquelmsmith feel free to ping me in person if you want to get this over the line together |
|
The test to make sure that this actually works was failing with: It seems to be not finding the new constraint that you added @frankh. I'm not certain how the migration you added works, is it expected that this test would be failing? I did add a commit to comment-out the test, but if it's not expected that it would be failing then prob don't want to merge this. Also the migration validation check is failing, but without a useful error message, so I'm not sure if that is expected or if something is wrong with the migration. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Problem
See: https://posthog.slack.com/archives/C043VJ93L3B/p1694100638757679
We have a plan to make dropped data recoverable: https://posthog.slack.com/archives/C0374DA782U/p1694197775137269
However it would be nice if for larger customers we made sure that their data was never dropped.
Changes
Adds a field to the organization model
never_drop_data. This field retrieved from the customer's billing response (added in https://github.com/PostHog/billing/pull/358). If true, then we never quota limit the org. However if they are over their limits, CS is notified via email.How did you test this code?
Added a test.